fix(sandbox): canonicalize HTTP request-targets before L7 policy evaluation by johntmyers · Pull Request #878 · NVIDIA/OpenShell

johntmyers · 2026-04-17T20:15:43Z

Summary

The L7 REST proxy evaluates OPA path rules against the raw HTTP request-target and forwards the raw header block byte-for-byte to the upstream. The upstream normalizes .., %2e%2e, and // before dispatching, so a compromised agent inside the sandbox can bypass any path-based allow rule — e.g. GET /public/../secret matches /public/** at the proxy but the upstream serves /secret.
The inference-routing detection (normalize_inference_path) had the same normalization gap: it only stripped scheme+authority and preserved dot-segments verbatim, so the same class of payload could also dodge inference routing.
This PR introduces a single URL-path canonicalizer used at every L7 enforcement site. The canonical path is both the input to OPA and the bytes written onto the wire — closing the parser-differential between the policy engine and the upstream.

closes OS-99

Changes

crates/openshell-sandbox/src/l7/path.rs (new)
- canonicalize_request_target(target, opts) -> Result<(CanonicalPath, Option<String> /* raw query */), CanonicalizeError>.
- Percent-decodes (uppercase hex on re-emit), resolves . / .. per RFC 3986 5.2.4 (rejects underflow rather than silently clamping to /), collapses doubled slashes, strips trailing ;params, handles origin-form and absolute-form targets, enforces a 4 KiB length bound, rejects fragments / raw non-ASCII / control bytes / encoded slashes (unless explicitly opted in per-endpoint via allow_encoded_slash).
- Returns a rewritten flag so callers know whether the outbound request line needs to be rebuilt.
crates/openshell-sandbox/src/l7/rest.rs
- parse_http_request canonicalizes the request-target before returning the L7Request. If the canonical form differs from the raw input, the request line in raw_header is rebuilt via rewrite_request_line_target so the upstream dispatches on the same bytes the policy evaluated.
- RestProvider now carries a CanonicalizeOptions field. Callers that need per-endpoint options construct via RestProvider::with_options(opts); default strict behavior via RestProvider::default().
- parse_query_params is now pub(crate) so proxy.rs can reuse it.
crates/openshell-sandbox/src/l7/relay.rs
- relay_rest builds its RestProvider from the endpoint's allow_encoded_slash, so L7 endpoints backed by upstreams that legitimately embed %2F (e.g. GitLab namespaced paths) can opt in per endpoint.
- relay_passthrough_with_credentials uses the strict default.
crates/openshell-sandbox/src/l7/mod.rs
- L7EndpointConfig gains allow_encoded_slash: bool (default false).
- parse_l7_config reads it from the Rego endpoint object.
crates/openshell-policy/src/lib.rs + proto/sandbox.proto
- NetworkEndpointDef (YAML), NetworkEndpoint (proto) gain the allow_encoded_slash field; to_proto copies it. Operators set allow_encoded_slash: true on an endpoint in YAML to opt in.
crates/openshell-sandbox/src/proxy.rs
- The forward (plain HTTP proxy) L7 evaluation site canonicalizes the request-target. The canonical form is reassigned to the outer path so the later rewrite_forward_request call writes canonical bytes to the upstream (closes the parser-differential for this site — previously only the L7 tunnel was closed). Non-canonical input is denied with 400 Bad Request and an OCSF NetworkActivity Fail event.
- normalize_inference_path is now a thin wrapper over the canonicalizer (falls back to the raw path on canonicalization error so the normal forward path can reject properly).
crates/openshell-sandbox/data/sandbox-policy.rego
- Comment documenting the invariant: input.request.path is pre-canonicalized, so rules must not attempt defensive matching against .. or %2e%2e.
architecture/sandbox.md
- Adds the new l7/path.rs module and notes the allow_encoded_slash per-endpoint opt-in.

Testing

cargo test -p openshell-sandbox — 524 lib + integration tests pass.
cargo test -p openshell-policy — 47 tests pass.
mise run pre-commit — passes (lint, format, license headers, rust tests).
Unit tests in l7::path::tests (24) cover dot segments, percent-encoded dot segments, double-slash collapse, encoded-slash reject/opt-in, null/control byte rejection, fragment rejection, absolute-form stripping, legitimate percent-encoded bytes round-tripping, mixed-case percent normalization, length bound, non-ASCII rejection, query-suffix separation, rewritten-flag semantics.
Integration tests in l7::rest::tests (9 new):
- parse_http_request_canonicalizes_target_and_rewrites_raw_header asserts both L7Request.target (what OPA sees) and raw_header (what the upstream receives) are canonical.
- parse_http_request_canonicalization_preserves_query_string, parse_http_request_leaves_canonical_input_byte_for_byte, parse_http_request_preserves_http_10_version_on_rewrite.
- parse_http_request_accepts_encoded_slash_when_endpoint_opts_in / _rejects_encoded_slash_by_default verify the allow_encoded_slash gate end-to-end.
- parse_http_request_rejects_traversal_above_root.
proxy::tests::test_rewrite_forward_request_uses_canonical_path_on_the_wire — regression test asserting that once the caller canonicalizes, rewrite_forward_request produces canonical bytes on the wire.
l7::tests::parse_l7_config_allow_encoded_slash_{defaults_false,opt_in} verify the YAML/proto/Rego wiring of the opt-in.
Regression tests for the documented bypass payloads:
- /public/../secret → /secret
- /public/%2e%2e/secret → /secret
- /public/%2E%2E%2Fsecret → EncodedSlashNotAllowed
- //public/../secret → /secret
- /public/./../secret → /secret
- /public/%00/secret → NullOrControlByte
- /.. → TraversalAboveRoot
Legitimate traffic continues to pass: /files/hello%20world.txt, /users/caf%C3%A9, /v1/chat/completions?stream=true, paths with : and @ in segments.

Trade-offs

%2F inside a segment is rejected by default. Operators who need it for artifact-registry-style APIs (e.g. GitLab namespaced project paths) set allow_encoded_slash: true on the endpoint in their YAML policy.
Requests that fail canonicalization are hard-rejected regardless of EnforcementMode::Audit — audit mode is for policy-tuning, not for tolerating protocol violations. Requests that succeed canonicalization are rewritten (if non-canonical) and then handled per the endpoint's enforcement mode as usual.
%20 and similar legitimate percent-encoded bytes are preserved in the canonical form (only unreserved pchars get decoded), so policy patterns that currently match /files/hello%20world.txt continue to work.

Checklist

Conventional Commits format
No secrets or credentials in diff
Scoped to the issue at hand (no unrelated refactors)
Architecture and rego documentation updated

…uation The L7 REST proxy evaluated OPA path rules against the raw request-target and forwarded the raw header block byte-for-byte to the upstream. The upstream normalized `..`, `%2e%2e`, and `//` before dispatching, so a compromised agent inside the sandbox could bypass any path-based allow rule by prefixing the blocked path with an allowed one (e.g. `GET /public/../secret` matches `/public/**` at the proxy but the upstream serves `/secret`). The inference-routing detection had the same normalization gap via `normalize_inference_path`, which only stripped scheme+authority and preserved dot-segments verbatim. - Add `crates/openshell-sandbox/src/l7/path.rs` with `canonicalize_request_target`. Percent-decodes (with uppercase hex re-emission), resolves `.` / `..` segments per RFC 3986 5.2.4, collapses doubled slashes, strips trailing `;params`, rejects fragments / raw non-ASCII / control bytes / encoded slashes (unless explicitly enabled per-endpoint), and returns a `rewritten` flag so callers know when to rewrite the outbound request line. - Wire the canonicalizer into `rest.rs::parse_http_request`. The canonical path is the `target` on `L7Request` that OPA sees, and when the canonical form differs from the raw input the request line is rebuilt in `raw_header` so the upstream dispatches on the exact bytes the policy evaluated. - Canonicalize the forward (plain HTTP proxy) path at `proxy.rs`'s second L7 evaluation site. A non-canonical request-target is rejected with 400 Bad Request and an OCSF `NetworkActivity` Fail event rather than silently passed through. - Replace the old `normalize_inference_path` body with a call to the canonicalizer. On canonicalization error the raw path is returned (so inference detection simply misses and the normal forward path handles and rejects). - Document the invariant in `sandbox-policy.rego`: `input.request.path` is pre-canonicalized so rules must not attempt defensive matching against `..` or `%2e%2e`. - Architecture doc (`architecture/sandbox.md`) lists the new module. - 24 new unit tests cover dot segments, percent-encoded dot segments, double-slash collapse, encoded-slash reject/opt-in, null/control byte rejection, legitimate percent-encoded bytes round-tripping, mixed-case percent normalization, fragment rejection, absolute-form stripping, empty / length-bounded / non-ASCII handling, and regression payloads for the specific bypasses above. Tracks OS-99.

copy-pr-bot · 2026-04-17T20:15:46Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

mjamiv · 2026-04-19T03:30:32Z

+1 on security merit. Path-based OPA rules are load-bearing for binary-scoped network policies in real deployments, and .. / %2e%2e / // bypass on the L7 path is an active concern for anyone using path-level egress allowlisting on inference proxies. Worth landing.

pimlock · 2026-04-20T17:58:48Z

From description:

Non-canonical requests are always hard-rejected regardless of EnforcementMode::Audit — audit mode is for policy-tuning, not for tolerating protocol violations.

From what I can tell, only requests that the canonicalization fails for are rejected, but non-canonical requests that pass canonicalization are accepted.

This would be more accurate: "Requests that fail canonizalication are hard-rejected regardless of EnforcementMode::Audit..."

pimlock · 2026-04-20T18:03:04Z

%2F inside a segment is rejected by default. Operators who need it for artifact-registry-style APIs can opt in per-endpoint via allow_encoded_slash: true (the plumbing is in place — endpoint config wire-up can land in a follow-up once there's a consumer).

I was looking for things this could potentially break. One example is GitLab that uses %2F for encoding paths (https://docs.gitlab.com/api/rest/#namespaced-paths).

That would mean mean that:

if I have a L4-only policy for GitLab -> it will work
if I switch to L7 (e.g. want to enforce things on more granular level) -> the request will be rejected if it contains %2F

pimlock · 2026-04-20T18:38:53Z

Security review — one finding I'd like to raise before merge, plus a note on tests.

Finding: `handle_forward_proxy` evaluates OPA on the canonical path but forwards the raw path to the upstream

The PR's stated invariant (l7/path.rs:4-12) is that the canonical path is both what OPA evaluates and the bytes written on the wire. That holds in l7/rest.rs:parse_http_request — which calls rewrite_request_line_target when canonical.rewritten is true — but is only half-implemented in proxy.rs:handle_forward_proxy.

Trace:

path extracted raw from parse_proxy_uri(target_uri) at proxy.rs:1984.
Inside the if let Some(l7_config) block at proxy.rs:2163-2202, the raw path is canonicalized into a new local canonical_path and fed to OPA via request_info.target. Policy sees canonical.
The canonical_path binding goes out of scope at the L7 block close around proxy.rs:2291. Only the outer raw path from step 1 is live after that point.
At proxy.rs:2523, rewrite_forward_request(buf, used, &path, …) is called with the raw path. That path is written byte-for-byte into the outbound request line at proxy.rs:1894 (output.extend_from_slice(path.as_bytes())).

Impact: For the plain-HTTP forward-proxy path, OPA decides on canonical_path while the upstream receives the raw path and re-normalizes independently. If upstream normalization matches this canonicalizer byte-for-byte, no harm. If it diverges on any edge (double-decoding, ;params handling, percent-slash treatment, different .. semantics), the parser-differential this PR is meant to close is still open for this call site.

Contrast with l7/rest.rs:171-179 where the same canonicalization is carried through to the wire via rewrite_request_line_target.

Fix options:

Option A (smaller): mark path as mut and reassign path = canon.path inside the Ok branch at proxy.rs:2167-2173 instead of binding a fresh canonical_path. The later rewrite_forward_request(buf, used, &path, …) then picks up the canonical form naturally.
Option B: call rewrite_request_line_target (or its forward-proxy equivalent) against buf inside the L7 block when canonical.rewritten is true. Mirrors what rest.rs does.

Either closes the finding.

Missing integration tests

All 24 new tests in l7::path::tests exercise canonicalize_request_target in isolation — none drive a full request through parse_http_request → rewrite_request_line_target, or through handle_forward_proxy → rewrite_forward_request, to assert on the bytes that actually reach the upstream. That assertion is what the PR's core invariant hinges on (what OPA sees == what the upstream receives), and it would have caught the gap above immediately.

Suggest adding at minimum:

A test that drives rest.rs:parse_http_request with a non-canonical input, inspects the returned L7Request.raw_header, and asserts the request line is canonical.
A test that drives handle_forward_proxy with a non-canonical input, captures what's written to a mock upstream, and asserts the same. This would fail against the current code.

…encoded_slash Addresses two review findings on the L7 path canonicalization change. 1. `handle_forward_proxy` previously evaluated OPA on the canonical path but wrote the raw path to the upstream via `rewrite_forward_request`, leaving the parser-differential open for plain-HTTP forward-proxy traffic even though it was closed for the L7 tunnel path. `path` is now reassigned to the canonical form inside the L7 block before the outbound write, so the bytes dispatched to the upstream match what OPA evaluated. Covered by a new regression test against `rewrite_forward_request`. 2. `allow_encoded_slash` is now wired through the YAML policy schema, the proto `NetworkEndpoint` message, the Rego endpoint config, and the `L7EndpointConfig` → `RestProvider` canonicalization options. Operators can opt in per-endpoint for upstreams like GitLab that embed `%2F` in namespaced resource paths; the default remains strict. The `RestProvider` gains a constructor `RestProvider::with_options` so `relay_rest` constructs a provider with per-endpoint options while `relay_passthrough_with_credentials` retains strict defaults. Integration tests: - `parse_http_request_canonicalizes_target_and_rewrites_raw_header` drives a non-canonical request through the parser and asserts both `L7Request.target` (what OPA evaluates) and `raw_header` (what the upstream receives) are canonical. - `parse_http_request_canonicalization_preserves_query_string`, `parse_http_request_leaves_canonical_input_byte_for_byte`, `parse_http_request_rejects_traversal_above_root`, `parse_http_request_preserves_http_10_version_on_rewrite`. - `parse_http_request_accepts_encoded_slash_when_endpoint_opts_in` / `_rejects_encoded_slash_by_default` verify the `allow_encoded_slash` gate. - `test_rewrite_forward_request_uses_canonical_path_on_the_wire` asserts the fix to the forward-proxy flow. - `parse_l7_config_allow_encoded_slash_{defaults_false,opt_in}` verify the YAML/Rego wiring.

johntmyers · 2026-04-20T21:43:17Z

Thanks for the careful review — all three points addressed in cb87e28. Replying inline so each fix maps back to what it's responding to.

From description:

Non-canonical requests are always hard-rejected regardless of EnforcementMode::Audit — audit mode is for policy-tuning, not for tolerating protocol violations.

From what I can tell, only requests that the canonicalization fails for are rejected, but non-canonical requests that pass canonicalization are accepted.

This would be more accurate: "Requests that fail canonizalication are hard-rejected regardless of EnforcementMode::Audit..."

Good catch — my wording conflated the two cases. Updated the PR description to "Requests that fail canonicalization are hard-rejected regardless of EnforcementMode::Audit...". Requests that canonicalize successfully to a non-canonical input (e.g. /a/./b → /a/b) are rewritten and continue through the normal enforcement path.

%2F inside a segment is rejected by default. Operators who need it for artifact-registry-style APIs can opt in per-endpoint via allow_encoded_slash: true (the plumbing is in place — endpoint config wire-up can land in a follow-up once there's a consumer).

I was looking for things this could potentially break. One example is GitLab that uses %2F for encoding paths [...]

That would mean mean that:

if I have a L4-only policy for GitLab -> it will work

if I switch to L7 (e.g. want to enforce things on more granular level) -> the request will be rejected if it contains %2F

Fair — "follow-up" was the wrong answer given you can hit this today. Wired through end to end:

NetworkEndpointDef (YAML) and NetworkEndpoint (proto) gain allow_encoded_slash: bool (default false).
L7EndpointConfig carries it; parse_l7_config reads it from the Rego endpoint object.
relay_rest constructs RestProvider::with_options(...) using the per-endpoint value so the parser honors the operator's setting.
Forward-proxy path in proxy.rs uses the same l7_config.allow_encoded_slash.

GitLab usage: set allow_encoded_slash: true on the endpoint in YAML and /api/v4/projects/group%2Fproject round-trips verbatim. Default-strict preserved; nothing changes for existing policies. Covered by parse_http_request_accepts_encoded_slash_when_endpoint_opts_in / _rejects_encoded_slash_by_default and parse_l7_config_allow_encoded_slash_{defaults_false,opt_in}.

Finding: handle_forward_proxy evaluates OPA on the canonical path but forwards the raw path to the upstream

[...]

Option A (smaller): mark path as mut and reassign path = canon.path inside the Ok branch at proxy.rs:2167-2173 instead of binding a fresh canonical_path. The later rewrite_forward_request(buf, used, &path, …) then picks up the canonical form naturally.

Confirmed the gap, thanks — this was real. Went with Option A:

path is now mut at the parse_proxy_uri destructuring.
Inside the L7 canonicalize Ok branch, path = canon.path so every downstream use (OPA input via request_info.target, the OCSF decision log, and critically rewrite_forward_request(buf, used, &path, ...)) sees the canonical form.
Non-canonical input continues to be rejected with 400 Bad Request + OCSF NetworkActivity::Fail.

Missing integration tests

All 24 new tests in l7::path::tests exercise canonicalize_request_target in isolation [...]

Also fair. Added end-to-end assertions:

l7::rest::tests::parse_http_request_canonicalizes_target_and_rewrites_raw_header — drives GET /public/../secret HTTP/1.1 through parse_http_request and asserts both L7Request.target (what OPA sees) and raw_header (what the upstream receives) are /secret. This is the core invariant assertion.
proxy::tests::test_rewrite_forward_request_uses_canonical_path_on_the_wire — the equivalent assertion for the forward-proxy flow. Fails against the prior code; passes with the fix above.
Plus rest-side tests for query-string preservation on rewrite, byte-for-byte pass-through when the input is already canonical, HTTP/1.0 version preservation on rewrite, traversal-above-root rejection, and the allow_encoded_slash opt-in gate.

524 sandbox + 47 policy tests pass. mise run pre-commit clean.

johntmyers requested a review from a team as a code owner April 17, 2026 20:15

johntmyers added topic:l7 Application-layer policy and inspection work topic:security Security issues labels Apr 17, 2026

johntmyers self-assigned this Apr 17, 2026

johntmyers added the test:e2e Requires end-to-end coverage label Apr 17, 2026

pimlock approved these changes Apr 20, 2026

View reviewed changes

johntmyers merged commit c960d48 into main Apr 21, 2026
11 checks passed

johntmyers deleted the fix/l7-path-canonicalization branch April 21, 2026 14:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sandbox): canonicalize HTTP request-targets before L7 policy evaluation#878

fix(sandbox): canonicalize HTTP request-targets before L7 policy evaluation#878
johntmyers merged 2 commits intomainfrom
fix/l7-path-canonicalization

johntmyers commented Apr 17, 2026 •

edited

Loading

Uh oh!

copy-pr-bot Bot commented Apr 17, 2026

Uh oh!

mjamiv commented Apr 19, 2026

Uh oh!

pimlock commented Apr 20, 2026

Uh oh!

pimlock commented Apr 20, 2026

Uh oh!

pimlock commented Apr 20, 2026

Uh oh!

johntmyers commented Apr 20, 2026

Finding: `handle_forward_proxy` evaluates OPA on the canonical path but forwards the raw path to the upstream

Missing integration tests

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

johntmyers commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Testing

Trade-offs

Checklist

Uh oh!

copy-pr-bot Bot commented Apr 17, 2026

Uh oh!

mjamiv commented Apr 19, 2026

Uh oh!

pimlock commented Apr 20, 2026

Uh oh!

pimlock commented Apr 20, 2026

Uh oh!

pimlock commented Apr 20, 2026

Finding: handle_forward_proxy evaluates OPA on the canonical path but forwards the raw path to the upstream

Missing integration tests

Uh oh!

johntmyers commented Apr 20, 2026

Finding: handle_forward_proxy evaluates OPA on the canonical path but forwards the raw path to the upstream

Missing integration tests

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

johntmyers commented Apr 17, 2026 •

edited

Loading

Finding: `handle_forward_proxy` evaluates OPA on the canonical path but forwards the raw path to the upstream

Finding: `handle_forward_proxy` evaluates OPA on the canonical path but forwards the raw path to the upstream